refactor: remove logger from install command by mwbrooks · Pull Request #367 · slackapi/slack-cli

mwbrooks · 2026-03-06T23:07:03Z

Changelog

N/A

Summary

This pull request is part 3 of removing internal/logger package and is focused on the the install command.

Impacts Deno deployed installations
Impacts Bolt or local installations

Test Steps

# Create an app
$ lack create my-app -t https://github.com/slack-samples/deno-starter-template
$ cd my-app/

# TEST: Deploy
$ lack deploy
# → Confirm no unexpected newlines or spacing issues
# → Note: Trigger creation has a double newline in production

# TEST: Re-Deploy
$ lack deploy
# → Confirm no unexpected newlines or spacing issues
# → Note: Trigger creation has a double newline in production
# → Note: Manifest warning home_team_only has a double newline in production

# TEST: Run
$ lack run
# → Confirm no unexpected newlines or spacing issues

# TEST: Re-Run
$ lack run
# → Confirm no unexpected newlines or spacing issues
# → Note: Manifest warning home_team_only has a double newline in production

# Cleanup
$ lack delete -f
$ lack delete -f
$ cd ../
$ rm -rf my-app/

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

codecov · 2026-03-06T23:12:51Z

Codecov Report

❌ Patch coverage is 81.96721% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.23%. Comparing base (0d9a1e2) to head (65e355c).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/pkg/apps/add.go	0.00%	7 Missing ⚠️
internal/pkg/apps/install.go	95.83%	2 Missing ⚠️
internal/pkg/platform/localserver.go	0.00%	1 Missing ⚠️
internal/pkg/platform/run.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #367      +/-   ##
==========================================
- Coverage   65.24%   65.23%   -0.01%     
==========================================
  Files         216      216              
  Lines       18026    17991      -35     
==========================================
- Hits        11761    11737      -24     
+ Misses       5176     5161      -15     
- Partials     1089     1093       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mwbrooks

Comments for the reviewers eyes

mwbrooks · 2026-03-06T23:08:38Z

cmd/app/add.go

-			switch event.Name {
-			case "app_install_manifest":
-				// Ignore this event and format manifest outputs in create/update events
-			case "app_install_manifest_create":


note: You can search for any of the following event names to see that the output is now display where the event was previous broadcast:

app_install_manifest_create

app_install_manifest_update

app_install_start

app_install_icon_success

app_install_icon_error

app_install_complete

mwbrooks · 2026-03-06T23:09:47Z

internal/pkg/apps/add.go

 	}
-	log.Data["teamName"] = *authSession.TeamName
-
-	log.Info("on_apps_add_init")


note: This event name was broadcast but never subscribed anywhere 🤦🏻

mwbrooks · 2026-03-06T23:10:21Z

internal/pkg/apps/add.go

 }

 // addAppLocally will add the app to the project's apps file with an empty AppID, TeamID, etc
-func addAppLocally(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger, app types.App) (types.App, error) {


note: diff doesn't show that log *logger.Logger was removed.

mwbrooks · 2026-03-06T23:10:44Z

internal/pkg/apps/add.go


 // addAppLocally will add the app to the project's apps file with an empty AppID, TeamID, etc
-func addAppLocally(ctx context.Context, clients *shared.ClientFactory, log *logger.Logger, app types.App) (types.App, error) {
-	log.Info("on_apps_add_local")


note: Another event that was broadcast with no subscribers.

mwbrooks · 2026-03-06T23:11:19Z

internal/pkg/apps/add.go

 		return installState, types.App{}, slackerror.Wrap(err, slackerror.ErrAppAdd)
 	}

-	log.Info("on_apps_add_remote_success")


note: another event that was broadcast with no subscribers 😿

mwbrooks · 2026-03-06T23:11:55Z

internal/pkg/apps/install.go

+		clients.IO.PrintInfo(ctx, false, "\n%s", style.Sectionf(style.TextSection{
+			Emoji: "books",
+			Text:  "App Manifest",
+			Secondary: []string{
+				fmt.Sprintf(`Updated app manifest for "%s" in "%s"`, slackManifest.DisplayInformation.Name, *authSession.TeamName),
+			},
+		}))


note: Example of replacing the event broadcast with the actual output.

mwbrooks · 2026-03-06T23:12:46Z

internal/pkg/apps/install.go

-	appManageURL := fmt.Sprintf("%s/apps", apiInterface.Host())
-	log.Data["appURL"] = fmt.Sprintf("%s%s", appManageURL, app.AppID)
-	log.Data["appName"] = manifest.DisplayInformation.Name


note: appURL was never output anywhere.

mwbrooks · 2026-03-06T23:57:48Z

internal/pkg/apps/install.go

+		_, _ = clients.IO.WriteOut().Write([]byte("\n" + style.Sectionf(style.TextSection{
+			Emoji: "books",
+			Text:  "App Manifest",
+			Secondary: []string{
+				fmt.Sprintf(`Updated app manifest for "%s" in "%s"`, slackManifest.DisplayInformation.Name, *authSession.TeamName),
+			},
+		})))


note: I'm using the identical clients.IO.WriteOut() syntax from cmd/ to avoid introducing formatting issues. The clients.IO.PrintInfo inserts newlines after the output, causing issues.

🌟 praise: Good call for scoping this change!

👾 ramble: I'd be interested in standardizing formats to avoid these special cases. We've talked about this before and I think these changes bring us so much closer to these realities!

🗣️ note: Outputting from the internal package might also be something to avoid ongoing. Preferring cmd and perhaps composing commands might be another pattern to use if we find this continues to be a duplicated pattern here.

👾 ramble: I'd be interested in standardizing formats to avoid these special cases. We've talked about this before and I think these changes bring us so much closer to these realities!

Agreed! While doing this work, I found myself thinking about how we can soon tackle improving our styling and formatting to avoid double-spaces. Hoping that comes soon!

🗣️ note: Outputting from the internal package might also be something to avoid ongoing. Preferring cmd and perhaps composing commands might be another pattern to use if we find this continues to be a duplicated pattern here.

I agree. I think a noble goal is to delete internal/pkg. The logic should either be moved to the command cmd/ or into reusable packages under internal/. This may naturally encourage output to be in cmd/.

zimeg

@mwbrooks LGTM! Once again I thank you so much for these changes! Onward with more reviews and merging these soon 🚢 💨

zimeg · 2026-03-09T22:26:25Z

internal/pkg/apps/install.go

+		_, _ = clients.IO.WriteOut().Write([]byte("\n" + style.Sectionf(style.TextSection{
+			Emoji: "books",
+			Text:  "App Manifest",
+			Secondary: []string{
+				fmt.Sprintf(`Updated app manifest for "%s" in "%s"`, slackManifest.DisplayInformation.Name, *authSession.TeamName),
+			},
+		})))


🌟 praise: Good call for scoping this change!

👾 ramble: I'd be interested in standardizing formats to avoid these special cases. We've talked about this before and I think these changes bring us so much closer to these realities!

🗣️ note: Outputting from the internal package might also be something to avoid ongoing. Preferring cmd and perhaps composing commands might be another pattern to use if we find this continues to be a duplicated pattern here.

zimeg · 2026-03-09T22:27:40Z

internal/pkg/apps/install.go

-		log.Info("app_install_manifest_update")
-		log.Info("on_update_app_install")


🪓 praise: This is so much easier for me to understand!

mwbrooks · 2026-03-09T23:47:29Z

@zimeg Thanks again for taking the time to review this PR! These aren't exciting PRs to review or test, but I hope it leads to better code that's more enjoyable to work with. 🍵

refactor: remove logger from app install, add, and function mocks

1b94ec1

mwbrooks added this to the Next Release milestone Mar 6, 2026

mwbrooks self-assigned this Mar 6, 2026

mwbrooks added code health M-T: Test improvements and anything that improves code health semver:patch Use on pull requests to describe the release version increment labels Mar 6, 2026

mwbrooks commented Mar 6, 2026

View reviewed changes

mwbrooks changed the title ~~refactor: remove logger from app install, add, and function mocks~~ refactor: remove logger from install command Mar 6, 2026

refactor: fix formatting

2d3de52

mwbrooks commented Mar 7, 2026

View reviewed changes

mwbrooks marked this pull request as ready for review March 7, 2026 00:08

mwbrooks requested a review from a team as a code owner March 7, 2026 00:08

zimeg approved these changes Mar 9, 2026

View reviewed changes

Merge branch 'main' into mwbrooks-chopping-the-log-3

65e355c

mwbrooks merged commit 017f8d3 into main Mar 9, 2026
7 checks passed

mwbrooks deleted the mwbrooks-chopping-the-log-3 branch March 9, 2026 23:47

		log.Info("app_install_manifest_update")
		log.Info("on_update_app_install")

Conversation

mwbrooks commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

Summary

Test Steps

Requirements

Uh oh!

codecov bot commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mwbrooks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwbrooks commented Mar 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mwbrooks commented Mar 6, 2026 •

edited

Loading

codecov bot commented Mar 6, 2026 •

edited

Loading